-
Notifications
You must be signed in to change notification settings - Fork 45
Feature/pub 1676 message annotations #2642
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple comments & suggestions but generally looks good 👍
will leave it to the docs team to approve.
8d7eb0c
to
ed9ed44
Compare
@m-hulbert for some reason the left nav menu gets squished when the content is too wide, despite the fact that it seems to wrap okay |
In 9582fcf I have added the error codes used by message annotations, which have been updated in ably/ably-common#307 And aligned in realtime in: https://github.com/ably/realtime/pull/7514 We need to decide on the name used for the channel rule in the website and can align the error codes, error messages used in realtime and the docs here accordingly. |
In 66a92ed I have renamed the "Mutable messages" channel rule to "Advanced message features" per ADR-144 (pending decision) and included the appropriate caveats for features that are not yet supported on mutable messages enabled channels. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Mike, this is comprehensive in terms of content - I just think shuffling the content around might help in understanding it a little quicker.
I know I've made various suggestions throughout, but in hindsight I wonder if the entire page should speak solely in terms of message summaries considering that is what we recommend. Then we have an independent section at the end that explains how/why you can subscribe to individual events. Otherwise it becomes quite complex to explain both instances everywhere. This would make some of my comments obsolete, but let me know what you think.
Also, the content squish is a regression in the MDX implementation which I've logged with the web team. It's related to codeblock scrolling/wrapping of long lines where we've added line numbering.
<Aside data-type="info"> | ||
When using message annotations, normal messages delivered to the [`subscribe()`](/docs/pub-sub#subscribe) listener will have an `action` of `message.create`. | ||
</Aside> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this should be part of the intro section, but more importantly clearly state that annotation (summaries) use the same listener as regular messages on the channel. WDYT?
<Aside data-type='note'> | ||
Individual annotation events are useful for activity feeds or detailed logging, but for maintaining UI state, summary events are generally more reliable and efficient. | ||
</Aside> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd pull this up into the intro of the section and out of the aside.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm rewriting this para. "More reliable" is not right (receiving raw annotations is perfectly reliable)
Agree with Simon's comment, otherwise this LGTM @m-hulbert, reads nicely, thanks :) |
0125c44
to
612c0c0
Compare
Made the update to the modes. I also updated the channel rule with the latest terminology, and rebased to fix the issue with the page widths, so it should be readable in the review app now. |
612c0c0
to
298a0ef
Compare
I've rebased this on main (somewhat painfully) and updated it for protocol v4. (Our docs will just unconditionally use protocol v4 message docs for annotations and updates, since by the time we release those features experimentally all 3 sdks which had support for them will be updated to v4). I didn't keep the previous commit structure; there were a lot of fixup commits that didn't cleanly apply to the commit they were fixing up given other ones inbetween. I squashed it down to one commit nominally written by @mschristensen , and added my changes as a new commit after that. |
4314f2b
to
769c246
Compare
855fb67
to
8ddcc34
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - thanks for taking the hit on the rebase @SimonWoolf
8ddcc34
to
2b00074
Compare
And remove references to `version` for now until we add the update/delete docs. Our docs will just unconditionally use protocol v4 message docs for annotations and updates, since by the time we release those features experimentally all 3 sdks which had support for them will be updated to v4.
Removed 'Publishing individual annotation events' section separate from 'annotation publish', I think that's confusing. Just mentioned that you can specify a data payload at the bottom of the publish section.
ddd65c2
to
cedc044
Compare
csharp: Timestamp | ||
|
||
Timestamp when the message was received by the Ably, as <span lang="default">milliseconds since the epoch</span><span lang="ruby">a @Time@ object</span><br>.__Type: <span lang="default">@Integer@</span><span lang="java">@Long Integer@</span><span lang="csharp">@DateTimeOffset@</span><span lang="ruby">@Time@</span><span lang="objc,swift">@NSDate@</span>__ | ||
Timestamp when the message was first received by the Ably, as <span lang="default">milliseconds since the epoch</span><span lang="ruby">a @Time@ object</span><br>.__Type: <span lang="default">@Integer@</span><span lang="java">@Long Integer@</span><span lang="csharp">@DateTimeOffset@</span><span lang="ruby">@Time@</span><span lang="objc,swift">@NSDate@</span>__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by the Ably?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realise this is already merged but I've made some suggestions
--- | ||
|
||
<Aside data-type='experimental'> | ||
Message annotations are currently Experimental. They are still in development and subject to rapid change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggest delete "rapid"
Message annotations are currently Experimental. They are still in development and subject to rapid change. | ||
</Aside> | ||
|
||
Message annotations enable clients to add metadata to existing messages on a channel. You can use annotations to implement features like: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really a fan of this characterisation. Annotations are fully-fledged messages, whereas saying "metadata" suggests that annotations would not themselves have content. I prefer to describe it as "annotations enable clients to append information to existing messages".
* **Content categorization** - tag messages with categories such as "important" or "urgent" | ||
* **Community moderation** - flag inappropriate content for review | ||
* **Read receipts** - mark messages as "read" or "delivered" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think here an example could be to add comments to a message - ie make it clear that annotations can have message content
* **Community moderation** - flag inappropriate content for review | ||
* **Read receipts** - mark messages as "read" or "delivered" | ||
|
||
When clients publish or delete an annotation, Ably automatically creates a [summary](#annotation-summaries) that provides an aggregated view of all annotations for that message. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.. for the associated message
Annotations can be enabled for a channel or channel namespace with the *Message annotations, updates, and deletes* channel rule. | ||
|
||
<Aside data-type='important'> | ||
When message annotations are enabled, messages are [persisted](/docs/storage-history/storage#all-message-persistence) regardless of whether or not persistence is enabled, in order to support the feature. This may increase your package cost. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"This may increase your package cost": this needs to be clearer. The "package" cost isn't increased. More usage is incurred because persisted messages are counted additionally for billing purposes. Perhaps: "Messages that are persisted incur additional cost"
|
||
## Subscribe to annotation summaries <a id="subscribe" /> | ||
|
||
The recommended way to receive annotation updates is through annotation summaries. These events provide a summary of the complete, current state of all annotations for a message whenever an annotation is published or deleted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't agree with this characterisation. Annotations are messages, but we're saying you're really not supposed to use that feature?
I also think this comment conflates a summary and a summary event.
I would prefer that we say "The simplest way to receive annotation updates is via annotation summaries. A message that has annotations also includes a summary which is a synopsis of certain details of all annotations for that message. Summaries are updated in response to further annotation events for that message, and summary changes are delivered by default to subscribing clients.
|
||
## Subscribe to individual annotation events <a id="individual-annotations"/> | ||
|
||
It is also possible to subscribe to individual annotation events, rather than annotation summaries. These are the emitted when [publishing](#publish) or [deleting](#delete) an annotation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is also possible to subscribe to individual annotation events, as distinct from annotation summaries
|
||
It is also possible to subscribe to individual annotation events, rather than annotation summaries. These are the emitted when [publishing](#publish) or [deleting](#delete) an annotation. | ||
|
||
Individual events can be useful for activity feeds or detailed logging, but generally, for most usecases, subscribed clients should rely on aggregated summaries. The aggregation of annotations for a message into a summary attached to the message is the primary benefit of using the annotations API; an app design oriented around every client needing to subscribe to raw annotation events may not be taking full advantage of the feature. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't agree with this. I think we should say that summaries are designed to address the most common usecases for annotations, so there is usually no need to subscribe to individual annotation events. However, subscribing to individual annotations is appropriate for usecases where the payload of individual annotation events is relevant to the application.
[Continuous history](/docs/storage-history/history#continuous-history) features are not supported. Be aware that if you are currently using continuous history and enable annotations, updates, and deletes, continuous history will no longer function. | ||
</Aside> | ||
|
||
1. Go to the [**Settings**](https://ably.com/accounts/any/apps/any/edit) tab of an app in your dashboard. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we describe dashboard settings like this do we also mention the possibility of configuring these via the control API or terraform?
|
||
The annotation type is a string of the format `namespace:summarization.version` where: | ||
|
||
* `namespace` is a string (e.g. `reactions`) that groups related annotations. Only annotations in the same namespace will be aggregated together to produce [summaries](#annotation-summaries). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
superfluous "together"
Description
Adds docs for the message annotations feature to the Messages section of the Pub/Sub product docs.
Includes:
type
specifiers the different summarization methods and their semanticsNot ready to merge yet until we are ready to make the release but opening as draft now for early review.
Checklist